fix missing zall TSNR2 columns #2368
Conversation
There was a problem hiding this comment.
Thanks, for the quick fix. It looks like this fixes the current issue. It does appear that this script has several baked-in assumptions about column ordering that could lead to more hiccups down the road. It may be better to e.g. use the new tsnr = np.where(np.char.startswith(np.array(tab.colnames), 'TSNR2_'))[0] to select all of the TSNR columns rather than just the last one. Other columns would have to be hard-coded though, which is presumably why that wasn't done initially.
This PR fixes the issue at hand, so I am happy to merge as-is, but I would appreciate a comment on the point above before we do so. That way our future selves have a better understanding of the choices.
I'm not worried about row changes. If the first million rows are identical and your code is only adding columns, I'm not sure how the rows could be impacted (unless there are NaN's, which would be a bigger problem than this change).
|
I agree that this PR still has fragile assumptions, especially about the location of the |
|
Thanks. We will address further robustness improvements in that PR. Merging this as good enough for now. |
This PR fixes the missing Kibo zall TSNR2_LYA and TSNR2_QSO columns reported in #2363 . This is required for making the final kibo/zcatalogs/v1/zall*.fits files.
The underlying issue is that
desispec.zcatalog.update_table_columnsmakes some assumptions about column order of the input files while trying to standardize the output column order. The input order changed in Kibo (for reasons I haven't investigated) which broke those assumptions.This is a minimal fix to get things to work for Kibo while still being backwards compatible with Iron etc. This update is only slightly less fragile than the original code, but while wrapping up Kibo I'm also trying to make the minimal working change and save deeper fixes for later.
The change: instead of assuming that TNSR2_LRG is the last TSNR2 column, check for for the last column that starts with TSNR2.
Test outputs using this branch are in $CFS/desi/spectro/redux/kibo/zcatalog/v1/zall-*-v1a.fits . These have the missing columns
TSNR2_QSOandTSNR2_LYAat the intended location (after the otherTSNR2*columns, and before the*_NSPECand*_PRIMARYcolumns). Due to the size of the files and Perlmutter being out today, I have not checked that every row of every other column is unchanged, but I did check the first million rows and they exactly match as expected.